Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for slash in group name #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bmteller
Copy link

Encode slash as %2F. If group name contained a / character then this would be interpreted by phoenix router as a different path segment. I'm not sure if this works across all browsers or network setups because maybe some browsers will unescape values in the path before submitting the path or middle boxes might rewrite the path to an unescaped form.

I'm also not sure what other characters would cause the functionality to break.

Alternatively, the UI should ban you from creating flags with characters it does not support. However, I'm not sure that this is realistic because the non-ui part of the project does not put any restriction on the characters that can be contained in a name.

Encode slash as %2F. If group name contained a / character then
this would be interpreted by the elixir as a different routing
segment. I'm not sure if this works across all browsers or
network setups because maybe some browsers will unescape values
in the path before submitting the path or middle boxes might
rewrite the path to an unescaped form.
@tompave
Copy link
Owner

tompave commented Sep 14, 2023

Thank you for using the package and the PR.

I'm not sure I can accept this, as I'd like to keep this part of the package simple.
I'm just afraid that once I start allowing for these exceptions, it becomes a slippery slope.

For reference, I documented this sort of limitation in the readme: https://github.com/tompave/fun_with_flags_ui#caveats

Why do you want to use / in group names?

@bmteller
Copy link
Author

bmteller commented Sep 14, 2023

in that case do you think it makes sense to update def validate in utils.ex to reject the / character like it already rejects the ? character for group names. these characters are already blocked from the flag name but not the group name. https://github.com/tompave/fun_with_flags_ui/blob/master/lib/fun_with_flags/ui/utils.ex#L161

@tompave
Copy link
Owner

tompave commented Sep 14, 2023

Yes, that would make sense. I'd be happy to accept a PR for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants